Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an archive task to GEFS system to archive files in HPSS #2895

Open
wants to merge 124 commits into
base: develop
Choose a base branch
from
Open

Add an archive task to GEFS system to archive files in HPSS #2895

wants to merge 124 commits into from
+215 −6

Conversation

AntonMFernando-NOAA
Copy link
Contributor

@AntonMFernando-NOAA AntonMFernando-NOAA commented Sep 7, 2024 •
edited
Loading

Description

Type of change

Change characteristics

  • Is this a breaking change (a change in existing functionality)? NO
  • Does this change require a documentation update? NO
  • Does this change require an update to any of the following submodules? NO

How has this been tested?

  • CI test in HERA and HERCULES

Checklist

  • Any dependent changes have been merged and published
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • I have made corresponding changes to the documentation if necessary

@AntonMFernando-NOAA AntonMFernando-NOAA changed the title Add an archive task to GEFS system to archive files to HPSS Add an archive task to GEFS system to archive files in HPSS Sep 7, 2024
EricSinsky-NOAA
EricSinsky-NOAA requested changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@EricSinsky-NOAA EricSinsky-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some initial suggestions.

parm/archive/gefs.yaml.j2 Outdated Show resolved Hide resolved
parm/archive/gefs.yaml.j2 Outdated Show resolved Hide resolved
@AntonMFernando-NOAA
Copy link
Contributor Author

AntonMFernando-NOAA commented Sep 18, 2024 •
edited
Loading

@EricSinsky-NOAA @WalterKolczynski-NOAA Ready for review. Advice whether the DO_EXTRACTVARS condition should be included in master_gefs.yaml.j2 (@EricSinsky-NOAA thinks it might not be necessary).

EricSinsky-NOAA
EricSinsky-NOAA requested changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@EricSinsky-NOAA EricSinsky-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some suggestions based on my testing of a GEFS replay test case.

parm/archive/gefs_extracted_atmos.yaml.j2 Outdated Show resolved Hide resolved
parm/archive/gefs_extracted_atmos.yaml.j2 Outdated Show resolved Hide resolved
parm/archive/gefs_extracted_atmos.yaml.j2 Outdated Show resolved Hide resolved
parm/archive/gefs_extracted_wave.yaml.j2 Outdated Show resolved Hide resolved
@AntonMFernando-NOAA
Copy link
Contributor Author

@EricSinsky-NOAA @WalterKolczynski-NOAA @DavidHuber-NOAA The PR is ready to review.

DavidHuber-NOAA
DavidHuber-NOAA reviewed Sep 19, 2024
View reviewed changes
parm/archive/master_gefs.yaml.j2
Comment on lines +8 to +11
{% include "gefs_extracted_atmos.yaml.j2" %}
{% include "gefs_extracted_ocean.yaml.j2" %}
{% include "gefs_extracted_ice.yaml.j2" %}
{% include "gefs_extracted_wave.yaml.j2" %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the extracted? Is this intended to be run only when running with DO_EXTRACTVARS=YES? If not, then I suggest dropping the _extracted.

EricSinsky-NOAA
EricSinsky-NOAA requested changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@EricSinsky-NOAA EricSinsky-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are getting close. Just a couple more suggestions.

parm/archive/gefs_extracted_ocean.yaml.j2 Outdated Show resolved Hide resolved
parm/archive/gefs_extracted_wave.yaml.j2 Outdated Show resolved Hide resolved
parm/archive/gefs_extracted_ocean.yaml.j2 Outdated Show resolved Hide resolved
@AntonMFernando-NOAA
Copy link
Contributor Author

@EricSinsky-NOAA @DavidHuber-NOAA Made all the suggested changes. If @WalterKolczynski-NOAA agrees with the renaming of gefs_extract_<product.yaml.j2> files I think this is ready for CI test.

EricSinsky-NOAA
EricSinsky-NOAA approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@EricSinsky-NOAA EricSinsky-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with regard to satisfying the current GEFS reforecast archiving requirements. If one of the GEFS archiving requirements changes, I think this can be addressed in a future PR. Thank you for establishing the GEFS archive task, Anton.

@AntonMFernando-NOAA
Copy link
Contributor Author

Looks good with regard to satisfying the current GEFS reforecast archiving requirements. If one of the GEFS archiving requirements changes, I think this can be addressed in a future PR. Thank you for establishing the GEFS archive task, Anton.

Thanks, Eric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidHuber-NOAA DavidHuber-NOAA DavidHuber-NOAA left review comments

@WalterKolczynski-NOAA WalterKolczynski-NOAA WalterKolczynski-NOAA requested changes

@EricSinsky-NOAA EricSinsky-NOAA EricSinsky-NOAA approved these changes

@aerorahul aerorahul Awaiting requested review from aerorahul

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AntonMFernando-NOAA @WalterKolczynski-NOAA @EricSinsky-NOAA @DavidHuber-NOAA @aerorahul